Skip to content

[TSD] Add FEATURES[‘ENABLE_EDXNOTES’] documentation - #26203

Closed
NeOneSoft wants to merge 7 commits into
openedx:masterfrom
eduNEXT:Gonzalo/Adding_FEATURES_ENABLE_EDXNOTES_documentation
Closed

[TSD] Add FEATURES[‘ENABLE_EDXNOTES’] documentation#26203
NeOneSoft wants to merge 7 commits into
openedx:masterfrom
eduNEXT:Gonzalo/Adding_FEATURES_ENABLE_EDXNOTES_documentation

Conversation

@NeOneSoft

Copy link
Copy Markdown
Contributor

Description

Add FEATURES[‘ENABLE_EDXNOTES’] documentation has the purpose to give some information about annotations
related with that feature toggle. This documentation does not affect the behavior of the feature and only has
useful information for edX community.

Reference:
https://openedx.atlassian.net/wiki/spaces/OpenOPS/pages/26182212/How+to+Get+edX+Notes+Running
https://blog.lawrencemcdaniel.com/open-edx-notes/

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 28, 2021
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @NeOneSoft! I've created OSPR-5572 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@natabene

Copy link
Copy Markdown
Contributor

@NeOneSoft Thank you! @robrap FYI

Comment thread lms/envs/common.py Outdated
# .. toggle_description: The edxnotes app is responsible for displaying parts of the Notes UI to students in different parts
# of the LMS, as well as figuring out whether Notes is enabled for a particular situation. The bulk of the actual work in
# storing the notes is done by a separate service (see the edx-notes-api repo).
# .. toggle_warnings: Requires the edx-notes-api service properly running and to have configured the django settings as well

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you specify which are those settings? So far, I think that they are EDXNOTES_INTERNAL_API and EDXNOTES_INTERNAL_API

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @morenol!!. Great I think EDXNOTES_INTERNAL_API and EDXNOTES_PUBLIC_API am I right?!

@robrap robrap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thank you so much for writing this up. I dropped a bunch of minor comments.

Comment thread lms/envs/common.py Outdated
# .. toggle_warnings: Requires the edx-notes-api service properly running and to have configured the django settings
# EDXNOTES_INTERNAL_API and EDXNOTES_PUBLIC_API
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread lms/envs/common.py Outdated
# EDXNOTES_INTERNAL_API and EDXNOTES_PUBLIC_API
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: None
# .. toggle_tickets: None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use the PR mentioned above: https://github.com/edx/edx-platform/pull/6321

Comment thread lms/envs/common.py Outdated
# course using the notes service. The bulk of the actual work in storing the notes is done by
# by a separate service(see the edx-notes-api repo).
# .. toggle_warnings: Requires the edx-notes-api service properly running and to have configured the django settings
# EDXNOTES_INTERNAL_API and EDXNOTES_PUBLIC_API

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding a warning to keep this in sync with Studio?

Suggested change
# EDXNOTES_INTERNAL_API and EDXNOTES_PUBLIC_API
# EDXNOTES_INTERNAL_API and EDXNOTES_PUBLIC_API. If you update this setting, also update in Studio.

Additionally, you could replace three lines of comments in cms here: https://github.com/edx/edx-platform/blob/2d7a8358efe563b6e1968f56229e4f72c9740857/cms/envs/common.py#L228-L231 with something like the following:

# See LMS annotations for details.

Comment thread lms/envs/common.py Outdated
@@ -434,6 +434,17 @@
'ENABLE_FOOTER_MOBILE_APP_LINKS': False,

# Let students save and manage their annotations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete this line. It is no longer needed now that we have this annotated.

@NeOneSoft

Copy link
Copy Markdown
Contributor Author

@robrap Thank you for your comments!I'll be updating them and attending any other suggestion!

@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. There were no failures.

@NeOneSoft NeOneSoft closed this Jan 29, 2021
@openedx-webhooks

Copy link
Copy Markdown

@NeOneSoft Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@NeOneSoft

Copy link
Copy Markdown
Contributor Author

PR closed on behalf of https://github.com/edx/edx-platform/pull/26256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants